e2e/libs/engine/crd.py (2)

Line range hint 1-1: Remove duplicate logging import

The logging module is imported twice, which causes a redefinition warning. The second import shadows the first one.

Apply this diff to fix the issue:

import logging

from kubernetes import client

from engine.base import Base
-from utility.utility import logging
+from utility.utility import logging as utility_logging

Also applies to: 6-6

🧰 Tools

🪛 Ruff (0.8.0)

6-6: Redefinition of unused logging from line 1

(F811)

---

14-15: Improved logging messages and conditional checks

The changes to logging messages and conditional checks improve code clarity. However, ensure consistent usage of the logging function throughout the file after fixing the duplicate import issue.

After fixing the duplicate import, update all logging calls to use the renamed import:

-            logging(f"Getting all engines of {volume_name}")
+            utility_logging(f"Getting all engines of {volume_name}")

Also applies to: 17-17, 20-20, 34-34, 38-38, 44-44, 46-46, 57-57

e2e/libs/volume/base.py (1)

84-92: LGTM! Consider adding docstrings for better documentation.

The abstract methods are well-defined with clear naming conventions. The addition of the rollback method completes the migration lifecycle handling.

Consider adding docstrings to describe the expected behavior and parameters:

 @abstractmethod
 def wait_for_volume_migration_to_be_ready(self, volume_name):
+    """Wait for volume to be ready for migration.
+    
+    Args:
+        volume_name (str): Name of the volume to check
+    """
     return NotImplemented

e2e/libs/keywords/volume_keywords.py (1)

271-281: LGTM! Consider standardizing log message format.

The implementation provides good logging and proper delegation to the Volume class. For consistency with other log messages in the file, consider standardizing the format:

-        logging(f'Waiting for volume {volume_name} migration to be ready')
+        logging(f'Waiting for volume {volume_name} to be ready for migration')

-        logging(f'Waiting for volume {volume_name} migration to node {node_name} complete')
+        logging(f'Waiting for volume {volume_name} to complete migration to node {node_name}')

-        logging(f'Waiting for volume {volume_name} migration to rollback to node {node_name}')
+        logging(f'Waiting for volume {volume_name} to rollback migration to node {node_name}')

e2e/libs/volume/crd.py (1)

331-343: Consider refactoring to reduce code duplication

The rollback check implementation is nearly identical to the migration completion check. Consider extracting the common logic into a helper method.

+    def _verify_engine_on_node(self, volume_name, node_name, operation_name):
+        verified = False
+        for i in range(self.retry_count):
+            logging(f"Waiting for volume {volume_name} {operation_name} to node {node_name} ({i}) ...")
+            try:
+                engines = self.engine.get_engines(volume_name)
+                verified = len(engines) == 1 and engines[0]['status']['endpoint'] and engines[0]['status']['ownerID'] == node_name
+                if verified:
+                    break
+            except Exception as e:
+                logging(f"Getting volume {volume_name} engines error: {e}")
+            time.sleep(self.retry_interval)
+        assert verified

     def wait_for_volume_migration_complete(self, volume_name, node_name):
-        complete = False
-        for i in range(self.retry_count):
-            logging(f"Waiting for volume {volume_name} migration to node {node_name} complete ({i}) ...")
-            try:
-                engines = self.engine.get_engines(volume_name)
-                complete = len(engines) == 1 and engines[0]['status']['endpoint'] and engines[0]['status']['ownerID'] == node_name
-                if complete:
-                    break
-            except Exception as e:
-                logging(f"Getting volume {volume_name} engines error: {e}")
-            time.sleep(self.retry_interval)
-        assert complete
+        self._verify_engine_on_node(volume_name, node_name, "migration")

     def wait_for_volume_migration_to_rollback(self, volume_name, node_name):
-        rollback = False
-        for i in range(self.retry_count):
-            logging(f"Waiting for volume {volume_name} migration to rollback to node {node_name} ({i}) ...")
-            try:
-                engines = self.engine.get_engines(volume_name)
-                rollback = len(engines) == 1 and engines[0]['status']['endpoint'] and engines[0]['status']['ownerID'] == node_name
-                if rollback:
-                    break
-            except Exception as e:
-                logging(f"Getting volume {volume_name} engines error: {e}")
-            time.sleep(self.retry_interval)
-        assert rollback
+        self._verify_engine_on_node(volume_name, node_name, "rollback")

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Check for concrete implementations of these methods
# Test: Search for concrete implementations of the migration methods
rg -l "def (wait_for_volume_migration_to_be_ready|wait_for_volume_migration_complete|wait_for_volume_migration_to_rollback)" --type python

#!/bin/bash
# Search for concrete implementations of the migration methods without file type restriction
rg -l "def (wait_for_volume_migration_to_be_ready|wait_for_volume_migration_complete|wait_for_volume_migration_to_rollback)"

# Also search for files that might contain these implementations
fd -e py | grep -i "volume.*rest"

#!/bin/bash
# Check the actual implementations in the found files
rg "def (wait_for_volume_migration_to_be_ready|wait_for_volume_migration_complete|wait_for_volume_migration_to_rollback)" -A 5 e2e/libs/volume/volume.py e2e/libs/volume/base.py e2e/libs/volume/crd.py e2e/libs/keywords/volume_keywords.py